Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract away the TestServer dependency from WebApplicationBuilder #60247

Closed
wants to merge 3 commits into from

Conversation

mkArtakMSFT
Copy link
Member

@mkArtakMSFT mkArtakMSFT commented Feb 7, 2025

Description

  • Replaces (avoiding breaking changes the TestServer dependency with a new ITestServer abstraction in the WebApplicationFactory.
  • Updated the WebApplicationFactory to depend on the ITestServer abstraction
  • Updated the GenerateMvcTestManifestTask to generate relative contentRoot paths in test manifests.

Validation

How do I know if this change didn't break something? Well, at first it did and many integration tests in the repo started failing. So these, naturally become a validation ground for this change.

Fixes #4892

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Feb 7, 2025
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch 2 times, most recently from 8cf15f6 to 9c0fb14 Compare February 12, 2025 05:35
@mkArtakMSFT mkArtakMSFT marked this pull request as ready for review February 12, 2025 05:35
@Copilot Copilot bot review requested due to automatic review settings February 12, 2025 05:35
@mkArtakMSFT mkArtakMSFT requested review from a team and halter73 as code owners February 12, 2025 05:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.

Files not reviewed (7)
  • src/Hosting/TestHost/src/PublicAPI.Unshipped.txt: Language not supported
  • src/Mvc/Mvc.Testing/src/PublicAPI.Unshipped.txt: Language not supported
  • src/Mvc/Mvc.Testing/src/Resources.resx: Language not supported
  • src/Mvc/test/Mvc.FunctionalTests/ApiExplorerTest.cs: Evaluated as low risk
  • src/Mvc/test/Mvc.FunctionalTests/Infrastructure/MvcTestFixture.cs: Evaluated as low risk
  • src/Identity/test/Identity.FunctionalTests/Infrastructure/ServerFactory.cs: Evaluated as low risk
  • src/Mvc/test/Mvc.FunctionalTests/ComponentRenderingFunctionalTests.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs:161

  • [nitpick] The method name 'Initialize' might be too generic. Consider renaming it to 'InitializeServer' for better clarity.
public void Initialize()

@mkArtakMSFT
Copy link
Member Author

mkArtakMSFT commented Feb 12, 2025

Still battling the issue introduced by the relative contentRoot change.
Looks like I've got the tests working now. Just some leftover issues to react / cleanup and the PR is ready to go.

@captainsafia
Copy link
Member

@mkArtakMSFT We'll need to do an API review for this since it includes a changes to the WAF interface.

Have you had the chance to use these changes with a Playwright-based setup?

@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch from e5aff71 to bfa3c20 Compare February 12, 2025 23:50
@mkArtakMSFT
Copy link
Member Author

Have you had the chance to use these changes with a Playwright-based setup?

Will do!

…ency with from in the WebApplicationFactory.

- Updated the WebApplicationFactory to depend on the ITestServer abstraction
- Fixed the public API declarations
- React to changes in the WebApplicationFactory
- Use solution-relative contentRoot path, when the path from metadata either is not available or doesn't exist.
- If none of the paths exist, use the AppContext.BaseDirectory instead.
- Fix tests with outdated expectations
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch from bfa3c20 to 1f62155 Compare February 13, 2025 00:57
@mkArtakMSFT
Copy link
Member Author

I have discovered an issue where the ITestServer creation happens through two different flows. I'm looking into whether this needs to be unified or not. Will update the PR once that's cleared.

@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch from c0a16cc to 1f62155 Compare February 14, 2025 22:47
…er as obsolete and introduce a new CreateTestServer method.
@mkArtakMSFT
Copy link
Member Author

Closing as we will be taking the alternative approach provided via #60635

@mkArtakMSFT mkArtakMSFT closed this Mar 6, 2025
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 6, 2025
mkArtakMSFT added a commit that referenced this pull request Mar 28, 2025
- Added `UseKestrel(...)` APIs to the WebApplicationFactory<T> type. The API configures the WAF, so that later during initialization it will use Kestrel, instead of a TestServer for WebHostBuilder-based applications.
- Two different overloads (described in the API Proposal #60758) are added
- Renamed the `EnsureServer()` private method to `StartServer()` and made it public, so that consumers can call it directly, without the need of creating a client, as in many situations customers didn't need a client to interact with.


This is an alternative design to enabling real server usage with WebApplicationFactory, which was considered here: #60247

Fixes #4892
@mkArtakMSFT mkArtakMSFT deleted the mkArtakMSFT/issue_4892 branch April 3, 2025 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve automated browser testing with real server
2 participants